-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Add support for updating existing Slack messages #4682
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add support for updating existing Slack messages #4682
Conversation
Signed-off-by: Kruchkov Alexandr <[email protected]>
38e21dc to
c4764e4
Compare
notify/metadata_store.go
Outdated
| @@ -0,0 +1,82 @@ | |||
| // Copyright 2024 Prometheus Team | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Copyright 2024 Prometheus Team | |
| // Copyright The Prometheus Authors |
Signed-off-by: Kruchkov Alexandr <[email protected]>
|
I'm not intending to block this change, but I do want to mention these limitations are the reasons @gotjosh and I didn't implement this in the past (although discussed):
We felt these limitations were too severe for the feature to make stable because it makes the feature behave too unpredictably. However, it sounds like you may also have a plan about how to address those limitations going forward? |
Thank you for the review! You're absolutely right about these limitations. I actually have a solution ready for both issues. But I want to ask what you'd prefer: Option A: Ship this as v1, iterate later Option B: Go all the way in this PR
I've tested Option B locally - metadata survives restarts and updates work correctly after restart. The changes are pretty clean. What's your preference? I'm happy to go either way - just want to align with how you prefer to merge features. |
|
Hi! Also not looking to block this, but just a drive by comment: We have an internal patch that adds a generic key/value store to the nflog as well. We've been running our production cluster with that patch for ~2 years now. We even use it for this exact purpose! One thing we found is that wiring it through all the notifiers is a little ugly. We ended up changing the signature of If there's interest, we'd be happy to upstream that. Our implementation is pretty much compatible with this PR - in the proto it's a |
Slack: Update existing messages instead of creating new ones
TL;DR
Before

After

Add
update_messageconfig option to Slack notifier. When enabled, updates existing messages in place instead of creating new ones for each alert status change.Current behavior creates multiple messages per alert group:
3 messages → clutters channel
With this PR:
1 message → clean channel
How
Implementation:
MetadataStoretracksmessage_tsandchannel_idper alert group (in-memory)chat.postMessage(new) andchat.update(existing)channel_idfrom first response (required by Slack API for updates)Flow:
Configuration
Requirements:
chat:writescopesend_resolved: truemust be setupdate_message: truemust be setTesting
Expected logs:
Limitations
Current implementation:
By design:
Backward Compatibility
✅ Opt-in feature, defaults to
false✅ No changes to existing configs
✅ No breaking changes